Skip to content

Conversation

@girum-air
Copy link
Contributor

No description provided.

@girum-air girum-air requested a review from rhunwicks October 2, 2025 11:53
@girum-air girum-air self-assigned this Oct 2, 2025
result = pd.DataFrame(data)
else:
result = data
# Handle unexpected types (like int, float)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that this will work. The current else is covering the situation where data has type pd.DataFrame. We want to end up in a situation where we have a regular dataframe to work on. What does pd.DataFrame(pd.DataFrame(data)) actually do.

Also, if the int label is passed as part of a list or series then it will still be part of the resulting dataframe.

Can't we solve this by just reversing the order of the if clauses and leaving everything else untouched:

    if isinstance(data, pd.DataFrame):
        result = data
    elif isinstance(data, (list, pd.Series)):
        result = pd.DataFrame(data)
    else:
        # Handle other types (like str, int, float)
        result = pd.DataFrame([data])

Then the result.map(str) will apply to a dataframe containing a column that contains an int, which will convert it to a str.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And similarly reverse the if statements for extracting the result from the dataframe to return.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@girum-air you might also want to add a pipelines_tests.test_utils.PrepareLookupTestCase that tests each of these input types (str, int, list, Series, DataFrame)

@rhunwicks rhunwicks merged commit f819089 into main Oct 3, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants